Skip to content

Update uhtml dependencies and logic#908

Merged
krausest merged 1 commit intokrausest:masterfrom
WebReflection:uhtml
Jul 3, 2021
Merged

Update uhtml dependencies and logic#908
krausest merged 1 commit intokrausest:masterfrom
WebReflection:uhtml

Conversation

@WebReflection
Copy link
Copy Markdown
Contributor

@WebReflection WebReflection commented Jun 30, 2021

This MR applies the following changes:

  • previous techniques are preserved
  • the 801 "issue" has been removed
  • new helpers landed as dependency update
  • some logic has been moved (as implementation detail) to the helper
  • events are attached directly to the element, like others without 801 do
  • no trickery beyond the events ... these are not a top level delegate, even if that would give more perf boost

Thanks for considering this MR 👋

@krausest krausest merged commit f68e0ab into krausest:master Jul 3, 2021
@krausest
Copy link
Copy Markdown
Owner

krausest commented Jul 3, 2021

Nice! What from the above explains the significant improvement for select row?
Screenshot from 2021-07-03 21-32-04

@WebReflection
Copy link
Copy Markdown
Contributor Author

@krausest apologies for the late reply. As the whole benchmark is based on a single id (one selected row per time, never more) the select row logic has been moved into "implementation detail", like it is for a few others libraries. The shared/common helper logic that drives the benchmark logic won't "move the world" on row selection changes.

I don't know if this needs to be flagged somehow, but 801 is gone (although I forgot to update the non-keyed bench too).

Happy to provide more details, if needed, and thanks for the merge. Non keyed might come soon too.

@Freak613
Copy link
Copy Markdown
Contributor

@krausest @WebReflection it looks like this implementation should be moved into #772 category, based on code from its dependency:
https://github.com/WebReflection/js-framework-benchmark-utils/blob/master/cjs/index.js#L108
We have a lot of other projects in this group. It is clear that current implementation is(or may be) idiomatic for this library, but current categories are the way it is. There is long discussion on the topic, so further suggestions and thoughts can be put there to keep others involved, while here we can discuss disagreements whether or not this implementation should be moved into this group.

Also, I feel that moving whole benchmark implementation into current repository will ease further reviews, without need to manually scan through each dependency for the actual code.

@ryansolid
Copy link
Copy Markdown
Contributor

ryansolid commented Jul 28, 2021

I didn't pick up on that. That repo of utils especially select row is clearly #772. It's hidden from the in repo implementation but it is also not library internals and clearly is tailored for the benchmark. It stashes the DOM element and toggles the classList directly with "danger".

@WebReflection
Copy link
Copy Markdown
Contributor Author

I feel that moving whole benchmark implementation into current repository will ease further reviews

Various libraries of mine use that very same dependency, but in more than one occasion I've stated that we should have shared benchmark logic across libraries so that all have the same underlying performance, and we'll measure, at that point, only libraries overhead.

I am OK with the #772 flag though, as I've already mentioned before.

I don't know if this needs to be flagged somehow, but 801 is gone

This benchmark does not allow multiple rows selection, so that looping through all of them makes no sense. Previous versions are still within the uhtml folder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants